[naga spv-out] Handle matCx2 in uniform buffers #8240
Conversation
jamienicol
force-pushed
the
matCx2-uniform-spv
branch
2 times, most recently
from
f407879 to
bbac5f3
Compare
|
@jimblandy I know we previously discussed my approach here of using alternative type declarations for uniform types, vs always declaring structs with a modified type, and you had a preference for the latter. After testing out both I found this approach much cleaner. There was less special casing as the rules for when we need to convert a type are more consistent, and being able to ignore stores altogether makes life much easier. (And FWIW I'm not convinced stores are implemented correctly in HLSL, but I still need to test that and file a bug if I'm right). I've tried to document that decision both in this PR and in the module-level comment.) |
Extend existing tests to use dynamic indexing of matrices' columns as well static indexing. Add new tests for arrays of matrices.
Two-row matrices in uniform buffers in Vulkan/SPIR-V's default "extended" (std140) layout do not adhere to WGSL/Naga IR's layout rules - each column is aligned to a minimum of 16 bytes rather than just the vector size. To work around this, we emit additional std140 compatible type declarations for each type used in a uniform buffer that requires one. Any two-row matrix struct member is decomposed into the containing struct as separate vectors for each column. Two-row matrices used directly as uniform buffers are wrapped in a struct with a vector member for each column, and arrays (of arrays, etc) of two-row matrices are declared as arrays (of arrays) of structs containing a vector member for each column. When loading such a value from a uniform buffer, we convert from the std140 compatible type to the regular type immediately after loading. Accesses of a uniform two-row matrix's columns using constant indices are rewritten to access the containing struct's vector members directly. Accesses using dynamic indices are implemented by loading and converting the matrix, extracting each column then `switch`ing on the index to select the correct column. We can now remove the expected failure annotation for the Vulkan backend on the uniform_input struct_layout test, as it now passes. We additionally make the hlsl_mat_cx*.wgsl snapshot tests run for the SPIR-V backend too, and remove the "hlsl" prefix from the name, as they are no longer just relevant for HLSL.
jamienicol
force-pushed
the
matCx2-uniform-spv
branch
from
bbac5f3 to
06e89f3
Compare
There was a problem hiding this comment.
Looks... well, I hesitate to say "good" not because of anything about the PR, but because the Cx2 handling is such a pain 😀. But seriously, thank you for taking care of this. This approach does seem less painful than the way that HLSL handles it.
I went looking for which tests are covering the case of accesses to struct members after a Cx2 matrix that is flattened into the struct. It seems like this is at least covered by the f16 test, but I don't see a test explicitly targeting this. I don't think it's necessary to add one, but it might be worth putting a note in the f16 test that it's providing this coverage.
|
|
||
| // https://github.com/gfx-rs/wgpu/issues/4371 | ||
| let failures = if storage_type == InputStorageType::Uniform && rows == 2 { | ||
| Backends::GL |
There was a problem hiding this comment.
It might be worth adding a comment where UNIFORM_INPUT is defined mentioning that some specific cases are disabled on the GL backend, since that's normally where an expected-failure would be declared.
| OpMemberName %49 2 "col2" | ||
| OpMemberName %49 3 "col3" | ||
| OpName %49 "std140_mat4x2<f32>" | ||
| OpName %50 "std140_array<mat4x2<f32>, 2>" |
There was a problem hiding this comment.
thought: Despite being technically valid, does having spaces in the debug names increase the risk of interoperability problems?
There was a problem hiding this comment.
Hrm, I'm not sure to be honest. I don't see why a driver or program would care, but you never know. Would you be more comfortable if I sanitized the names, or just remove them altogether? Or should we land as is and wait and see if we see any issues?
There was a problem hiding this comment.
I guess ideally we'd live in this promised land and could easily format type names for SPIRV. Presumably based on the SPIRV-tools FriendlyNameMapper.
There was a problem hiding this comment.
I think it's fine to leave as-is and wait until an issue comes up. The debug stuff should not be turned on in default configs so the risk is limited.
There was a problem hiding this comment.
Adding spv.debug = true to this might make it easier to inspect the results (thus more likely any regressions get caught during review).
naga/src/back/spv/writer.rs
Outdated
| block, | ||
| Instruction::switch( | ||
| column_index_param_id, | ||
| cases.last().unwrap().label_id, |
There was a problem hiding this comment.
Is this assuming BoundsCheckPolicy::Restrict?
There was a problem hiding this comment.
yes it was. I've pushed 9925f12 which handles the different policies
hehe.
I think an explicit tests makes sense now you mention it. It's easy enough to write. Done in 85656cc |
|
@andyleiserson I assume you're happy with the changes since you pre-approved? I think the only possibly contentious one was the bounds checking. @jimblandy anything to add? If not I'll squash and merge this tomorrow morning |
Connections
SPIR-V fix for #4371 (leave open for GLSL)
Description
Vulkan's default uniform buffer "extended layout" (std140) defines matrices in terms of arrays of their vector type, and defines arrays to have an alignment rounded up to a multiple of 16. WGSL/Naga IR defines matrices to have the same alignment as their vector type, not rounded up. For a matCx2 this means Naga expects each column to have an alignment of 8, whereas Vulkan expects 16.
To fix this, for each matCx2 struct member in a uniform buffer we decompose matrix such that each column is a vector member of the containing struct. For matrices used directly as uniform buffers, we create a struct that contains a vector member for each column. For arrays of matrices, we declare an array of a struct containing a vector member for each column.
We only use these alternative type declarations for uniform buffers, and when loading a value of such a type we convert it to the original type. This is in contrast to the approach used in the HLSL backend where all structs containing matCx2 have a modified layout. This allows the HLSL backend to avoid having to convert a struct after loading one, but it still must convert matrices or arrays of matrices when loading from uniform buffers, or when accessing values of these types that are struct members in any address space. The approach taken here means we consistently must convert two-row matrices, or arrays or structs containing two-row matrices, but only when loading from uniform buffers. It also allows us to ignore stores altogether, as uniform buffers are read only.
Testing
Extended struct_layout tests to handle additional cases, and remove expected fail for vulkan. Made existing hlsl_mat_cx[23].wgsl snapshot tests run on SPIR-V too.
Squash or Rebase?
Squash (making sure commit message is tidy)
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.